fix(cose): reject undecodable CBOR values in the unprotected header - #411
Conversation
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. The part that earns this is the discriminator, not the recursion.
Matching on type(value) is object rather than allow-listing the types a header may legitimately hold is the choice that makes the guard safe to put in front of every caller. cbor2's break marker is a bare object() and nothing a normal decode returns is, so the check cannot reject a manifest that decodes correctly, and it does not need revisiting when cbor2's tag support learns a new type. The docstring says exactly that, which is why I did not have to work it out from the code.
What I ran, in a clean venv on the fork head be71671:
pytest tests/: 1553 passed, 6 skipped.mypy src/agent_manifest: no issues in 26 source files.pytest tests/test_cose.pywithsrc/agent_manifest/_cose.pyreverted to main: 4 failed,test_unprotected_header_sentinel_is_checked_at_decode,
test_attach_unprotected_rejects_a_caller_supplied_unencodable_value,
test_reject_cbor_sentinels_recurses_into_cbortag_and_set,
test_unprotected_header_sentinel_nested_in_tag_or_set_is_checked_at_decode. All four pass with the fix, so the tests are guarding the change rather than describing it.
That 1553 figure is the other half of the evidence: a new refusal added to a shared decode path is only safe if it fires on nothing that already worked, and it fires on nothing.
Repo CI: 14 distinct checks, all green including Fuzz changed code and Security scan. The only red entry is Policy: Awaiting maintainer review, which this approval clears.
One thing to expect, not a change request. This PR and #407 both add to CHANGELOG.md at line 17, so whichever lands first conflicts the other. I am merging this one now; #407 will need its changelog entry rebased and that is a mechanical fix, not a review round. #373's entry is further down the file and should be unaffected.
carloshvp
left a comment
There was a problem hiding this comment.
Approved on exact head be71671.
Independent validation in an isolated checkout:
- Full Python suite: 1,553 passed, 6 skipped.
- mypy: no issues in 26 source files.
- Bandit: clean.
- git diff --check: clean.
- Baseline regression: current main leaks cbor2.CBOREncodeError for the recorded fuzz reproducer; this head rejects it as the documented CoseStructureError.
- Reviewed direct and nested sentinels across mappings, keys, arrays, tuples, sets, frozensets, and CBORTag payloads, caller-supplied unencodable values, and decoder nesting behavior.
All current CI checks are green, including ClusterFuzzLite, security scan, CodeQL, lint, type checking, and the platform test matrix.
What
Reject a malformed CBOR value (cbor2's internal break-marker sentinel) in the unprotected header before it can crash
attach_receipt/attach_attestation/attach_approvalson re-encode.Why
fuzz_cosefound an input where the unprotected header decodes fine but contains a value cbor2 can't re-encode.attach_unprotected()copies that header and callscbor2.dumps(), which raisesCBOREncodeErroran exception outside the documentedCoseErrorcontract, soexcept CoseErrordoesn't catch it.Spec impact
None
Test plan
pytest -vpassesmypy src/agent_manifestpassesruff check src/ tests/passesCHANGELOG.mdupdatedDCO
All commits in this PR are signed off (
git commit -s). By submitting this PR I certify the Developer Certificate of Origin.